Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(accessibility): Update editor packages & use hiddenLabel for Icon Button #1257

Merged
merged 1 commit into from
Dec 27, 2023
Merged

fix(accessibility): Update editor packages & use hiddenLabel for Icon Button #1257

merged 1 commit into from
Dec 27, 2023

Conversation

xrutayisire
Copy link
Collaborator

@xrutayisire xrutayisire commented Dec 27, 2023 •
edited
Loading

Context

The Solution

  • Update Button following editor-ui refactoring with secondary being color=grey and tertiary being color=dark
  • Remove usage of ButtonGroup in BlankSlateActions since className is not allowed anymore and ButtonGroup is overkill of our usage (discussed that with Alex)
  • Remove className for HoverCard button since we just want the button to be 100% and it's possible with sx
  • Add hiddenLabel prop instead of data-cy when used for IconButton
  • Update e2e tests acocrdingly

Impact / Dependencies

  • Accessibility is improved thanks to the hiddenLabel

Checklist before requesting a review

  • I hereby declare my code ready for review.
  • If it is a critical feature, I have added tests.
  • The CI is successful.
  • If there could backward compatibility issues, it has been discussed and planned.

@linear Linear
Copy link

linear bot commented Dec 27, 2023

DT-1874 AADev, I can add accessibility labels to @prismicio/editor-ui's <IconButton>

@vercel Vercel
Copy link

vercel bot commented Dec 27, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
slice-machine ✅ Ready (Inspect) Visit Preview Dec 27, 2023 9:09pm

angeloashmore
angeloashmore requested changes Dec 27, 2023
View reviewed changes
Copy link
Member

@angeloashmore angeloashmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with some suggested label changes. I suggested different labels to reflect how they should be announced rather than tested.

After those changes, we should be good to merge. 👍

packages/slice-machine/src/components/SideNav/SideNavEnvironmentSelector.tsx Outdated
@@ -87,7 +87,7 @@ export const SideNavEnvironmentSelector: FC<SideNavEnvironmentSelectorProps> = (
<IconButton
icon={<LoginIcon className={styles.loginIcon} />}
onClick={onLogInClick}
data-cy="environment-login-icon-button"
hiddenLabel="environment login icon button"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested label change: "Log in to enable environments"

Hidden labels should act as text labels rather than descriptions of the element itself ("button" shouldn't be used in this case).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're 100% right, I forgot the purpose of this with e2e tests!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

packages/slice-machine/src/components/SideNav/SideNavEnvironmentSelector.tsx Outdated
<IconButton icon="unfoldMore" data-cy="environment-dropdown-button" />
<IconButton
icon="unfoldMore"
hiddenLabel="environment dropdown button"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested label change: "Select environment"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

packages/slice-machine/src/components/Window/Window.tsx Outdated
<IconButton
icon="add"
onClick={onAddNewTab}
hiddenLabel="add tab button"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested label change: "Add new tab"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

packages/slice-machine/src/features/customTypes/EditDropdown.tsx Outdated
Comment on lines 75 to 82
<Button
loading={isCustomTypeBeingConverted}
startIcon="moreVert"
variant="secondary"
color="grey"
data-testid="editDropdown"
/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an IconButton instead? If so:

Suggested label: "Actions" or "Custom type actions"

(I'm not sure if this component is reused with page types. If so, we need to label it correctly.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're right, IconButton works. I guess it was before the refactoring that allowed variant="solid".

Done for both

packages/slice-machine/src/features/slices/sliceCards/SharedSliceCard.tsx Outdated
@@ -163,7 +163,7 @@ export const SharedSliceCard: FC<SharedSliceCardProps> = (props) => {
) : action.type === "menu" ? (
<DropdownMenu modal>
<DropdownMenuTrigger disabled={disabled}>
<IconButton data-cy="slice-action-icon" icon="moreVert" />
<IconButton hiddenLabel="slice action icon" icon="moreVert" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested label change: "Actions" or "Slice actions"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@xrutayisire xrutayisire force-pushed the xru/update-editor-packages-hidden-label-icon-button branch from 69c5d33 to 87ab4de Compare December 27, 2023 21:04
@xrutayisire xrutayisire merged commit 1b9bef7 into main Dec 27, 2023
33 checks passed
@xrutayisire xrutayisire deleted the xru/update-editor-packages-hidden-label-icon-button branch December 27, 2023 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angeloashmore angeloashmore angeloashmore requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xrutayisire @angeloashmore